Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

B5 Responsive Forms #35519

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

B5 Responsive Forms #35519

wants to merge 12 commits into from

Conversation

millerdev
Copy link
Contributor

@millerdev millerdev commented Dec 12, 2024

Most HQ forms are designed to use a "horizontal" layout, saving vertical space on large screens. Despite the fact that the form-horizontal class was removed, horizontal forms are still a documented concept in Bootstrap 5. The label and field classes HQ code has always used in Bootstrap 3 and 5 are responsive, making form layout adjust automatically and comfortably to different screen sizes (although sometimes forms don't work well on small screens on Bootstrap 3 for other reasons).

This PR introduces extension classes to encapsulate styling differences between Bootstrap 3 and 5, allowing code that uses them to remain unchanged during the transition. This makes the 3-to-5 migration simpler and easier because it requires less manual code changes. The style and migration guides have been updated for Bootstrap 5, including HTML and Crispy forms. Bootstrap 3 HTML form examples were not updated since those appear to be legacy. Hopefully we will not add many new B3 forms, but I am happy to update those examples as well if it would be useful.

Extensions classes could be used in other places as well. For example form row styles where class="row mb-3" becomes something like class="form-row". This allows styling tweaks to be applied in one central place (the extension) rather than requiring widespread edits to HTML.

Aside: mb-3 literally means margin bottom ..., which sounds a lot like an inline style. Would it not be better to encapsulate styling choices like that inside classes with semantic meaning rather than referencing cryptic abbreviated names that directly correspond to CSS properties?

🐡 Review by commit.

[B5] Wide screen layout

image

[B5] Narrow screen layout

image

Safety Assurance

Safety story

Tested locally and on staging.

Automated test coverage

No.

QA Plan

TODO

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Make it easier to associate label with field on wide screens.
Restore large-screen (lg) column widths used with Bootstrap 3.
Tweak medium-screen (md) column widths to improve consistency.
Should reduce the overhead of future site-wide style changes such as when upgrading Bootstrap.
It is not longer required to update BooleanFields to use BootstrapCheckboxInput since responsive horizontal forms have been restored. Indeed, when the label value is changed to an empty string (label="") it adds awkward vertical space resulting in odd layout unless the form is also properly updated to use CheckboxField (in many cases this step is missed).

BootstrapCheckboxInput may optionally be used if a (new) label value is added to the left, in addition to moving the previous label value to inline_label in the widget and updating the form layout to use CheckboxField. Otherwise the default BooleanField configuration is preferred.

One form was converted back to the simpler version of BooleanField (as well as restoring proper alignment of form actions) as an example in this commit. Other instances exist and may be converted in the future.
@millerdev millerdev added the product/invisible Change has no end-user visible impact label Dec 12, 2024
Copy link
Contributor

@orangejenny orangejenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is beautiful.

BootstrapCheckboxInput may optionally be used if a (new) label value is added to the left, in addition to moving the previous label value to inline_label in the widget and updating the form layout to use CheckboxField. Otherwise the default BooleanField configuration is preferred.

Is there any benefit to using BootstrapCheckboxInput? If there isn't a clear benefit, I think we should aim to deprecate it.

@@ -14,9 +14,9 @@
from crispy_forms.utils import flatatt, get_template_pack, render_field

CSS_LABEL_CLASS = 'col-xs-12 col-sm-4 col-md-4 col-lg-2'
CSS_LABEL_CLASS_BOOTSTRAP5 = 'col-xs-12 col-sm-4 col-md-3 col-lg-2'
CSS_LABEL_CLASS_BOOTSTRAP5 = 'field-label'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a major improvement.

@@ -70,12 +69,9 @@ class ConnectionSettingsForm(forms.ModelForm):
required=False,
)
skip_cert_verify = forms.BooleanField(
label="",
label=_('Skip certificate verification'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is so much nicer.

context.update({
'formactions': self,
'fields_output': fields_html,
'offsets': offsets,
'offsets': '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: can offsets be removed altogether form this dict?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mabye, I'll take another look. I'm not sure how many places use (require?) it. It's hard to search for because it's a common word. I also need to check if crispy-boostrap3to5 needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orangejenny
Copy link
Contributor

Coming back to this because I forgot to ask about testing. I see QA is TODO - what are you thinking there? I don't think we need to test every single form in HQ, but I do think it would be good to smoke test areas that use crispy heavily. Off the top of my head, messaging, users, locations, scheduled reports, and maybe project settings are the areas I think have the heaviest & most complex crispy usage.

@millerdev
Copy link
Contributor Author

Is there any benefit to using BootstrapCheckboxInput?

Yes. It's useful if you want a label on the left (field-label) + label after the checkbox. This layout:

image

I see QA is TODO - what are you thinking there? I don't think we need to test every single form in HQ, but I do think it would be good to smoke test areas that use crispy heavily. Off the top of my head, messaging, users, locations, scheduled reports, and maybe project settings are the areas I think have the heaviest & most complex crispy usage.

I agree, and that sounds like a good plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants